Skip to content

Preserve mtime of files when preserve_mtime flag is set - #192

Closed
kellycampbell wants to merge 1 commit into
bazelbuild:masterfrom
kellycampbell:preserve-file-mtime
Closed

kellycampbell wants to merge 1 commit into
bazelbuild:masterfrom
kellycampbell:preserve-file-mtime

Conversation

@kellycampbell

Copy link
Copy Markdown

Similar to #155, we need to have our content files served with a timestamp representing when they were last modified or built.

@kellycampbell
kellycampbell requested a review from aiuto as a code owner June 23, 2020 16:40
@aiuto

aiuto commented Jul 6, 2020

Copy link
Copy Markdown
Collaborator

This has be a distinct flag. In #155 we determined that preserve mtimes of files in included tarballs. will not break repeatable builts. Using the mtime of files has reproducibility problems.

@aiuto aiuto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has be a distinct flag. In #155 we determined that preserve mtimes of files in included tarballs. will not break repeatable builts. Using the mtime of files has reproducibility problems.

@aiuto aiuto left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the discussion shorter.

  1. The flag should be named unsafe_use_filesystem_mtimes The 'unsafe' is a warning. because this is a foot-gun w.r.t. local vs. remote build.
  2. It needs integration tests. I strongly suggest you wait for me to merge #196 before writing those

@kellycampbell

kellycampbell commented Jul 14, 2020

Copy link
Copy Markdown
Author

My first path of action and still my preference would be to make this able to use the build stamp vars and the mtime arg, but I found that there wasn't really a straightforward way to get those from the bazel API for .bzl files without parsing out the stamp files myself. Perhaps I just didn't look in the right place. Also, the current usage of the mtime arg is only applied to the overall tar file's time, not the files inside.

@aiuto

aiuto commented Jul 14, 2020

Copy link
Copy Markdown
Collaborator

That is certainly different than what this PR does. What if there was a magical syntax like mtime = "VOLATILE_BUILD_TIME"?
This would make the target non-reproducible, because every time you build you would get a new result. OTOH, that might be what you want.

@kellycampbell

Copy link
Copy Markdown
Author

That is certainly different than what this PR does. What if there was a magical syntax like mtime = "VOLATILE_BUILD_TIME"?

Isn't that one of the purposes for the existing build stamping in Bazel, e.g. with BUILD_TIMESTAMP?

This would make the target non-reproducible, because every time you build you would get a new result. OTOH, that might be what you want.

Reproducible in the exact down to the byte form is not required. The main thing needed is for the files served via http from the new image should be the same or newer timestamps compared to the previously built and deployed image so the last-modified and etag headers are useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants